Skip to content

Account for stored pointer offsets in dereference checks#2024

Draft
karoliineh wants to merge 4 commits into
masterfrom
memsafety-ptr-deref
Draft

Account for stored pointer offsets in dereference checks#2024
karoliineh wants to merge 4 commits into
masterfrom
memsafety-ptr-deref

Conversation

@karoliineh
Copy link
Copy Markdown
Member

This is on top of #2017 based on #2017 (comment).

Changes:

  • Adds the regression from Fix memOutOfBounds handling of negative pointer offsets #2017 (comment) that exposed the unsoundness
  • Fixes dereference bound checks to include offsets already encoded in the pointer's points-to address, instead of only looking at the offset introduced by the current dereference expression
  • Refactors the new offset-handling logic into helper functions for readability
  • Simplifies BinOp handling by passing the original expression through the checks instead of destructuring and reconstructing it
    • I initially removed the match on BinOp from check_binop_exp altogether because the only place where it is called from is within a match case on BinOp ((PlusPI | MinusPI | IndexPI), ...), but decided to keep it just in case for future, so that it is not called from a wrong place and if it is, would notify with an error message.

This is still not an ideal code structure, as evidenced by the need for comments like:

(* Explicit pointer arithmetic is checked separately by check_binop_exp; do not add it here again. *)

Obviously the overall analysis workflow is a bit non-straightforward if we have to leave comments like this. But that is an issue for another cleanup PR, at least this should fix the unsoundness itself.

@karoliineh karoliineh added this to the SV-COMP 2027 milestone May 7, 2026
@karoliineh karoliineh self-assigned this May 7, 2026
@karoliineh karoliineh added bug unsound sv-comp SV-COMP (analyses, results), witnesses pr-dependency Depends or builds on another PR, which should be merged before labels May 7, 2026
@karoliineh karoliineh requested a review from michael-schwarz May 7, 2026 13:37
@karoliineh karoliineh marked this pull request as draft May 7, 2026 14:22
Base automatically changed from memsafety-neg-offset to master May 7, 2026 14:57
@karoliineh karoliineh marked this pull request as ready for review May 7, 2026 14:58
@karoliineh karoliineh marked this pull request as draft May 7, 2026 14:59
@karoliineh
Copy link
Copy Markdown
Member Author

I've been going back-and-forth with marking this as a draft and ready because I was unsure if this should be merged before the cleanup or after. For now, the fix will be applied during a cleanup that will be similar to the PR #1864, using access events for the memOutOfBounds analysis as was done for the useAfterFree analysis. So, most likely this PR will be replaced by a more extensive one and thus, review effort should not be wasted on this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug pr-dependency Depends or builds on another PR, which should be merged before sv-comp SV-COMP (analyses, results), witnesses unsound

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant